remove video transcript enabled flag by muhammad-ammar · Pull Request #17923 · openedx/openedx-platform

muhammad-ammar · 2018-04-11T10:30:52Z

With these changes Transcripts column on Video Uploads page will be enabled by default.

SANDBOX

edx-status-bot · 2018-04-11T11:11:36Z

Your PR has finished running tests.

edx-status-bot · 2018-04-11T14:51:57Z

Your PR has finished running tests.

edx-status-bot · 2018-04-12T06:53:11Z

Your PR has finished running tests.

edx-status-bot · 2018-04-13T07:35:32Z

Your PR has finished running tests.

edx-status-bot · 2018-04-23T13:53:28Z

Your PR has finished running tests.

edx-status-bot · 2018-04-24T12:42:33Z

Your PR has finished running tests.

edx-status-bot · 2018-04-25T15:12:32Z

Your PR has finished running tests.

edx-status-bot · 2018-04-26T04:45:00Z

Your PR has finished running tests.

muhammad-ammar · 2018-04-26T14:10:22Z

@mushtaqak This is waiting for your review

mushtaqak

@muhammad-ammar This is looking great. I have left a few questions and feedback.

Also, I think, openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled could/may be removed from test_video_handlers.py and lms/djangoapps/mobile_api/video_outlines/tests.py

mushtaqak · 2018-04-27T07:29:16Z

common/lib/xmodule/xmodule/video_module/video_module.py

    subs_filename,
    get_transcript_for_video
 )
-from .transcripts_model_utils import (


I think we can remove is_val_transcript_feature_enabled_for_course method from transcript_model_utils.py since it is not used anymore now.

mushtaqak · 2018-04-27T07:45:05Z

cms/djangoapps/contentstore/views/tests/test_transcript_settings.py

+        response = self.client.get(self.view_url, content_type='application/json')
        self.assertEqual(response.status_code, 405)

    def test_404_with_feature_disabled(self):


This test should be removed as well.

mushtaqak · 2018-04-27T07:56:50Z

cms/djangoapps/contentstore/views/tests/test_transcript_settings.py

        response = self.client.post(transcript_delete_url)
        self.assertEqual(response.status_code, 405)

-    def test_404_with_feature_disabled(self):


name of the class should be renamed from TranscriptUploadTest to TranscriptDeleteTest and

@patch( 'openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True) )

should be removed from the class as well.

mushtaqak · 2018-04-27T07:58:49Z

cms/djangoapps/contentstore/views/tests/test_transcript_settings.py

@@ -504,16 +480,6 @@ def test_405_with_not_allowed_request_method(self):
        response = self.client.post(transcript_delete_url)


Also let's make this consistent with other tests (self.view_url) as we have done for other views

this is out of scope of this PR :)

NVM, I found that this is required for has_studio_write_access permission used in delete handler.

mushtaqak · 2018-04-27T09:59:31Z

cms/djangoapps/contentstore/views/transcript_settings.py

-    video_transcripts_enabled = VideoTranscriptEnabledFlag.feature_enabled(course_key)
    # User needs to have studio write access for this course.
-    if not video_transcripts_enabled or not has_studio_write_access(request.user, course_key):
+    if not has_studio_write_access(request.user, course_key):


why do we need to check write access here ?

so that not everyone can delete the transcript

mushtaqak · 2018-04-27T10:01:48Z

cms/static/js/spec/views/previous_video_upload_list_spec.js

                    var view = new PreviousVideoUploadListView({
                        collection: collection,
                        videoHandlerUrl: videoHandlerUrl,
+                        transcriptAvailableLanguages: [],


Why are we adding these attributes now ?

muhammad-ammar · 2018-04-27T12:00:12Z

@mushtaqak feedback addressed.

edx-status-bot · 2018-04-27T12:05:37Z

Your PR has finished running tests.

mushtaqak · 2018-04-30T08:35:59Z

lms/djangoapps/courseware/tests/test_video_handlers.py

-        Mock(return_value=False),
-    )
    @patch('xmodule.video_module.VideoModule.get_transcript', Mock(side_effect=NotFoundError))
    def test_download_fallback_transcript_feature_disabled(self):


Do we need this test now ? or it's docstring need to be update?

let me check this

Deleted as I don't think we need these now.

mushtaqak · 2018-04-30T08:38:17Z

lms/djangoapps/mobile_api/video_outlines/tests.py

-    )
    @patch(
        'xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages',
        Mock(return_value=['uk']),


Is this test test_val_transcript_feature_disabled needed now? or it's docstring need to be update?

let me check this

Deleted as I don't think we need these now.

mushtaqak

Looks good except a couple of questions, otherwise 👍

muhammad-ammar · 2018-04-30T11:01:14Z

@mushtaqak I have rebased and squashed the PR. Will merge it as soon as build is gree.

edx-status-bot · 2018-04-30T11:36:30Z

Your PR has finished running tests.

muhammad-ammar force-pushed the ammar/EDUCATOR-1759-delete-transcript-from-advance-tab branch from 028a928 to c675b8c Compare April 11, 2018 11:07

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from dab7440 to 6b7a72a Compare April 11, 2018 14:12

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from 6b7a72a to b6c27a6 Compare April 12, 2018 06:11

muhammad-ammar requested review from Qubad786 and mushtaqak April 12, 2018 07:30

muhammad-ammar force-pushed the ammar/EDUCATOR-1759-delete-transcript-from-advance-tab branch from 6e1784f to 6784ee7 Compare April 12, 2018 19:54

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from b6c27a6 to 0f34796 Compare April 13, 2018 06:57

muhammad-ammar force-pushed the ammar/EDUCATOR-1759-delete-transcript-from-advance-tab branch from 6784ee7 to f2b499e Compare April 23, 2018 12:54

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from 0f34796 to d95fb31 Compare April 23, 2018 13:20

muhammad-ammar force-pushed the ammar/EDUCATOR-1759-delete-transcript-from-advance-tab branch from f2b499e to 82d0968 Compare April 23, 2018 13:45

muhammad-ammar force-pushed the ammar/EDUCATOR-1759-delete-transcript-from-advance-tab branch from 82d0968 to d5758bd Compare April 24, 2018 11:50

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from d95fb31 to a1e5931 Compare April 24, 2018 11:54

muhammad-ammar changed the base branch from ammar/EDUCATOR-1759-delete-transcript-from-advance-tab to transcripts-phase-2 April 25, 2018 13:27

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from a1e5931 to dbec3aa Compare April 25, 2018 14:23

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from dbec3aa to cc8d288 Compare April 26, 2018 03:59

mushtaqak suggested changes Apr 27, 2018

View reviewed changes

mushtaqak reviewed Apr 30, 2018

View reviewed changes

mushtaqak approved these changes Apr 30, 2018

View reviewed changes

remove video transcript enabled flag

aaaafee

muhammad-ammar force-pushed the ammar/remove-video-transcript-enabled-flag branch from 6a8ea29 to aaaafee Compare April 30, 2018 10:59

muhammad-ammar merged commit 16b0070 into transcripts-phase-2 Apr 30, 2018

UmanShahzad deleted the ammar/remove-video-transcript-enabled-flag branch May 9, 2018 20:42

		@@ -504,16 +480,6 @@ def test_405_with_not_allowed_request_method(self):
		response = self.client.post(transcript_delete_url)

Conversation

muhammad-ammar commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edx-status-bot commented Apr 11, 2018

Uh oh!

edx-status-bot commented Apr 11, 2018

Uh oh!

edx-status-bot commented Apr 12, 2018

Uh oh!

edx-status-bot commented Apr 13, 2018

Uh oh!

edx-status-bot commented Apr 23, 2018

Uh oh!

edx-status-bot commented Apr 24, 2018

Uh oh!

edx-status-bot commented Apr 25, 2018

Uh oh!

edx-status-bot commented Apr 26, 2018

Uh oh!

muhammad-ammar commented Apr 26, 2018

Uh oh!

mushtaqak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhammad-ammar commented Apr 27, 2018

Uh oh!

edx-status-bot commented Apr 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mushtaqak left a comment

Choose a reason for hiding this comment

Uh oh!

muhammad-ammar commented Apr 30, 2018

Uh oh!

edx-status-bot commented Apr 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

muhammad-ammar commented Apr 11, 2018 •

edited

Loading